Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not set empty classes and support option wrapped class names #1085

Merged
merged 16 commits into from
Apr 29, 2020

Conversation

liquidnya
Copy link
Contributor

@liquidnya liquidnya commented Apr 16, 2020

Hello,
this is a follow-up PR of #927 (which resolved #926).

E.g. The following view

html! {
    <div>{ "Example" }</div>
}

results in (as of version 0.14.3):
image

Fixes: #936

@jstarry
Copy link
Member

jstarry commented Apr 17, 2020

Thanks! Do you mind adding a testcase as well to make sure this doesn't happen again?

@liquidnya
Copy link
Contributor Author

I added test cases that check with hasAttribute("class") (https://developer.mozilla.org/en-US/docs/Web/API/Element/hasAttribute) if the className was set or not.
I tested the test case with geckodriver 0.26.0.

@liquidnya
Copy link
Contributor Author

I have found, that yew::virtual_dom::vtag::tests::supports_svg fails if the class is set to "" when the ancestor is empty.
The error output is:

wasm-bindgen: imported JS function that was not marked as `catch` threw an error: setting getter-only property "className"

className for SVG elements is read-only and deprecated, see https://developer.mozilla.org/en-US/docs/Web/API/SVGElement#Properties.
It also says Authors are advised to use Element.classList instead.
SVG Elements have the class property: https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/class.

I will quickly add a test-case, which will fail for now.

@liquidnya
Copy link
Contributor Author

There are multiple possibilities:

  1. revert back to using setProperty("class", ...) for all nodes
  2. use the classList.value property (sets the list to the given value) for all nodes https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/value
  3. a) use setProperty("class", ...) only for SvgElements
    b) use the classList.value property only for SvgElements

All of the above possibilities require different browser minimum versions.

@liquidnya
Copy link
Contributor Author

I reverted the change to use the className property (the class is set using setProperty("class", ...)).
Therefore the changes in this PR are now only fixing the issues of setting class="" and adding test cases.

@liquidnya liquidnya changed the title Set classes with className and do not set empty classes when creating elements in DOM Do not set empty classes when creating elements in DOM Apr 18, 2020
src/virtual_dom/vtag.rs Outdated Show resolved Hide resolved
@wathiede wathiede mentioned this pull request Apr 20, 2020
@jstarry
Copy link
Member

jstarry commented Apr 20, 2020

@LiquidBlock this looks good :) I was hoping you would treat "class" as an attribute and I wasn't disappointed!

I'm now thinking we could take this one step further! What if we remove classes from VTag and instead just add "class" to attributes?

I think we would need to remove the add_class and add_classes methods from VTag but I think that's totally fine. And set_classes could set "class" in attributes.

@liquidnya
Copy link
Contributor Author

I think we would need to remove the add_class and add_classes methods from VTag but I think that's totally fine. And set_classes could set "class" in attributes.

add_class is used in

Alignment::Left => vtag.add_class("text-left"),
Alignment::Center => vtag.add_class("text-center"),
Alignment::Right => vtag.add_class("text-right"),
and in
Alignment::Left => vtag.add_class("text-left"),
Alignment::Center => vtag.add_class("text-center"),
Alignment::Right => vtag.add_class("text-right"),

@liquidnya
Copy link
Contributor Author

I'm now thinking we could take this one step further! What if we remove classes from VTag and instead just add "class" to attributes?

That is a good idea to remove a lot of "duplicate" code and changing the implementation.
The classes set by the html! macro would be converted to Classes and then converted to String, which would then be inserted into the Attributes (HashMap).

add_class could be implemented by checking if the class is not already contained in the String and then appending it to the String. This would take O(n) complexity (amortized average, where n is the count of classes already in the class string), repeated calls would require quadratic time.
Keeping the IndexSet (contained in Classes) in memory (in VTag) results in O(1) (amortized average) complexity.

@jstarry
Copy link
Member

jstarry commented Apr 25, 2020

I think we should get rid of add_class. It just adds unnecessary complexity. If you want to do fun things with classes, you can use Classes directly and then call set_classes

@liquidnya
Copy link
Contributor Author

I dont know why it says, that i requested a review from hgzimmerman.
I am sorry, this was not intentional, maybe i clicked something by accident.

@liquidnya
Copy link
Contributor Author

I dont know why it says, that i requested a review from hgzimmerman.
I am sorry, this was not intentional, maybe i clicked something by accident.

I guess it happened automatically, since yew-router/examples/guide/src/markdown.rs is owned by hgzimmerman.

@liquidnya
Copy link
Contributor Author

I removed add_class and add_classes.
set_classes now sets the attribute directly.
I further added fn classes(&self) -> &str to VTag, which is a convenience method for accessing the attribute with something like vtag.attributes.get("class").unwrap_or("").

I was not sure how to fix the markdown examples:

Alignment::Left => vtag.add_class("text-left"),
Alignment::Center => vtag.add_class("text-center"),
Alignment::Right => vtag.add_class("text-right"),

and in

Alignment::Left => vtag.add_class("text-left"),
Alignment::Center => vtag.add_class("text-center"),
Alignment::Right => vtag.add_class("text-right"),

I added a method in those example, which parses the class string into Classes, adds the classes and sets those classes back to the VTag: https://github.com/yewstack/yew/pull/1085/files#diff-3e5d43901a65fa6fb6f5cff21489390dR7-R17
I guess that this is not the best solution, but i do not really know how to rewrite that example without investing a lot of time.

Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Much cleaner :) I'm wondering now if we should remove set_classes. What do you think?

yew-macro/src/html_tree/html_tag/mod.rs Outdated Show resolved Hide resolved
yew/src/virtual_dom/vtag.rs Outdated Show resolved Hide resolved
@hgzimmerman
Copy link
Member

The guide example for the router is getting removed in another open PR, so no worries. Glad to see the new CODEOWNERS file doing it's job.

Add possibility to use different types in the class tuple
Add convertion to Classes from Option<T> where T: AsRef<str>
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the way this turned out! Great work. Just a few small nits and then we can get this in :)

yew-macro/src/html_tree/html_tag/mod.rs Outdated Show resolved Hide resolved
yew-macro/src/html_tree/html_tag/mod.rs Outdated Show resolved Hide resolved
yew/src/virtual_dom/vtag.rs Outdated Show resolved Hide resolved
yew/src/virtual_dom/vtag.rs Show resolved Hide resolved
@liquidnya
Copy link
Contributor Author

@jstarry thank you for the detailed code review.
I pushed an overhaul.

@liquidnya
Copy link
Contributor Author

The beta job of Travis failed with: (https://travis-ci.com/github/yewstack/yew/jobs/324520704)

The command "tar -xzf geckodriver-v0.26.0-linux64.tar.gz" failed and exited with 2 during .

Should the title of this PR be updated to signify that also optional classes are possible now?

@jstarry jstarry changed the title Do not set empty classes when creating elements in DOM Do not set empty classes and support option wrapped class names Apr 29, 2020
@jstarry
Copy link
Member

jstarry commented Apr 29, 2020

The beta job of Travis failed

That's fine, just CI flakiness :/

Should the title of this PR be updated to signify that also optional classes are possible now?

Yes, I just updated!

@jstarry thank you for the detailed code review.
I pushed an overhaul.

Looks excellent, thank you for bearing with my feedback.

@jstarry jstarry merged commit d421192 into yewstack:master Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants